Add lock mode to query builder #4316
Conversation
|
|
||
| $query .= ($this->sqlParts['from'] ? ' FROM ' . implode(', ', $this->getFromClauses()) : '') | ||
| $query .= $databasePlatform->appendLockHint($from, $this->lockMode) |
There was a problem hiding this comment.
An potential issue here is if:
- Platform is MsSQL/SqlAnywhere
- Lock mode is set
- From clause is empty
An invalid query will be generated. Should this be considered user error (lock mode without a from is nonsensical) or should the code avoid adding the lock statement?
| */ | ||
| public function setLockMode(int $lockMode) | ||
| { | ||
| $this->lockMode = $lockMode; |
There was a problem hiding this comment.
The ORM query builder includes defensive coding around not allowing locks to be set outside of a transaction (they would be immediately released), should that be added here?
I lean towards no given the warning comments at the top of the class that it is the user's responsibility to use this class to generate valid queries.
carnage
changed the title
|
This is ready for review/merge now |
|
Since this functionality is highly dependent on the underlying platform, it will need cross-platform functional tests first of all. As far as I understand, it's challenging to test locking scenarios in a single-threaded environment, so it's enough to just execute the queries with all supported locking modes on all supported platforms and make sure that they are valid. Additionally, please provide an automated test in a free form (see #4279 for example) that describes the intended behavior of the new API. Also, note that since |
|
Not quite sure what you mean by an automated test in a free form? Do you just mean a script which connects to (eg MySQL) executes a query with a lock; sleeps for 10 secs then quits such that you could run two in parallel manually to show it works? I take it that there are no plans for a |
Yes, because you cannot run two parallel processes in a functional test. But the script should not only work with MySQL but with all supported platforms.
No. Please retarget to |
…e for queries on db platforms which don't append a hint
|
This feature looks amazing. @carnage I am not sure, is the work done? |
|
@morozov should I ask why was this PR closed? This improvement is imo good. |
|
@vaclavvanik the PR got closed because I removed the branch. I'll retarget it against |
|
@morozov ok, no problem :-) |
|
@carnage do you want to finish this PR? If no I would like to continue. |
|
Hi, Please feel free to bring it up to scratch to get it merged, just leave my original credit in :) the one thing missing was a "free form" test to demonstrate it working and any new work to resolve conflicts and get the build to pass. I was working on this as part of a client project which got terminated abruptly and I haven't had the time to come back to it. |
|
Closing due to the lack of progress. |
This is a prototype of adding the ability to add locks when building a query using the query builder in a platform agnostic way. For the most part, this mimics the way that it is done in the ORM query builder.
Summary